Some more fixes for issues found by afl/asan (#500)

* Fix uninitialized warning in lowranceusr

* Fix stack buffer overlow in lowranceusr

%15.10f may produce more characters than what fits into the
buffer of 32 bytes. Use std::isnan to avoid the sprintf.

==2133227==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff9bab5b20 at pc 0x7fc83fd95865 bp 0x7fff9bab57e0 sp 0x7fff9bab4f70
WRITE of size 38 at 0x7fff9bab5b20 thread T0
    #0 0x7fc83fd95864 in vsprintf (/lib64/libasan.so.5+0x9e864)
    #1 0x7fc83fd95d1e in sprintf (/lib64/libasan.so.5+0x9ed1e)
    #2 0x767bbb in lowranceusr_parse_waypt /home/gpsbabel/gpsbabel.git/lowranceusr.cc:813
    #3 0x77be54 in lowranceusr_parse_waypts /home/gpsbabel/gpsbabel.git/lowranceusr.cc:1116
    #4 0x781151 in data_read /home/gpsbabel/gpsbabel.git/lowranceusr.cc:1658
    #5 0xc7ac71 in run /home/gpsbabel/gpsbabel.git/main.cc:339
    #6 0x4cdd62 in main /home/gpsbabel/gpsbabel.git/main.cc:707
    #7 0x7fc83f29d1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
    #8 0x4cef7d in _start (/home/gpsbabel/gpsbabel.git/gpsbabel+0x4cef7d)

* Ensure string is terminated mps_readstr in mapsource

When EOF or buffer limit is reached, the return buffer might not be
zero-terminated. So add termination at the end of the buffer and read
max sz - 1 bytes from file instead of sz bytes.

==2145172==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd2ba34e00 at pc 0x7f005812dbbd bp 0x7ffd2ba34860 sp 0x7ffd2ba34008
READ of size 1028 at 0x7ffd2ba34e00 thread T0
    #0 0x7f005812dbbc  (/lib64/libasan.so.5+0x67bbc)
    #1 0x557c62 in QString::fromUtf8(char const*, int) /usr/include/qt5/QtCore/qstring.h:572
    #2 0x557c62 in QString::operator=(char const*) /usr/include/qt5/QtCore/qstring.h:706
    #3 0x557c62 in mps_waypoint_r /home/gpsbabel/gpsbabel.git/mapsource.cc:571
    #4 0x55edd4 in mps_read /home/gpsbabel/gpsbabel.git/mapsource.cc:1675
    #5 0xc7adb1 in run /home/gpsbabel/gpsbabel.git/main.cc:339
    #6 0x4ce082 in main /home/gpsbabel/gpsbabel.git/main.cc:707
    #7 0x7f005766c1a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
    #8 0x4cf29d in _start (/home/gpsbabel/gpsbabel.git/gpsbabel+0x4cf29d)

Address 0x7ffd2ba34e00 is located in stack of thread T0 at offset 1344 in frame
    #0 0x556e8f in mps_waypoint_r /home/gpsbabel/gpsbabel.git/mapsource.cc:507

  This frame has 6 object(s):
    [48, 56) 'wptdesc' (line 538)
    [80, 88) '<unknown>'
    [112, 120) '<unknown>'
    [144, 152) '<unknown>'
    [176, 276) 'tbuf' (line 508)
    [320, 1344) 'wptname' (line 509) <== Memory access at offset 1344 overflows this variable

* Fix off-by-one list access in pcx reader

* Prevent stack based buffer overflow in gdb reader

* Prevent global buffer overflow in garmin_fit

The message_def array is 16 bytes long, make sure local_id stays
within range, same as in other places in garmin_fit.

Found by afl which managed to create a call to fit_parse_data_message
with a header value of 0x1f:

==2927747==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000010a3bd8 at pc 0x000000a91bba bp 0x7ffd82584a90 sp 0x7ffd82584a80
READ of size 4 at 0x0000010a3bd8 thread T0
    #0 0xa91bb9 in fit_parse_data /home/gpsbabel/gpsbabel.git/garmin_fit.cc:549
    #1 0xa92ae3 in fit_parse_data_message /home/gpsbabel/gpsbabel.git/garmin_fit.cc:821
    #2 0xa92ae3 in fit_parse_record /home/gpsbabel/gpsbabel.git/garmin_fit.cc:864
    #3 0xa92ae3 in fit_read /home/gpsbabel/gpsbabel.git/garmin_fit.cc:884
    #4 0xc7cdb1 in run /home/gpsbabel/gpsbabel.git/main.cc:339
    #5 0x4ce142 in main /home/gpsbabel/gpsbabel.git/main.cc:707
    #6 0x7f4f3b2651a2 in __libc_start_main (/lib64/libc.so.6+0x271a2)
    #7 0x4cf35d in _start (/home/gpsbabel/gpsbabel.git/gpsbabel+0x4cf35d)

diff --git a/garmin_fit.cc b/garmin_fit.cc
index f43c750f7c0869f22c1924043c6eca520a7adaff..9189f83b612cf755ace1a2486a7835889cf5919b 100644 (file)
--- a/garmin_fit.cc
+++ b/garmin_fit.cc
@@ -816,7 +816,7 @@ fit_parse_data(fit_message_def* def, int time_offset)
 static void
 fit_parse_data_message(uint8_t header)
 {
-  int local_id = header & 0x1f;
+  int local_id = header & 0x0f;
   fit_message_def* def = &fit_data.message_def[local_id];
   fit_parse_data(def, 0);
 }

diff --git a/gdb.cc b/gdb.cc
index f1f2395438feb962186a6458a026671c2d325568..c7ca33874be4713bc1d16fe71d11cb84a707162b 100644 (file)
--- a/gdb.cc
+++ b/gdb.cc
@@ -401,6 +401,8 @@ read_file_header()
   }
 
   reclen = FREAD_i32;
+  is_fatal((reclen + 1 > int(sizeof(buf))),
+           MYNAME ": Invalid record length\n");
   (void) FREAD(buf, reclen + 1);
   if (global_opts.verbose_status > 0) {
     const char* name = buf+2;

diff --git a/lowranceusr.cc b/lowranceusr.cc
index 740e9a6b07f7b07067ea2003be4e0c7d51e0e4bd..dd9164bae7ccdfd1f1c3407d24fc0a5a412a1c13 100644 (file)
--- a/lowranceusr.cc
+++ b/lowranceusr.cc
@@ -809,10 +809,7 @@ lowranceusr_parse_waypt(Waypoint* wpt_tmp, int object_num_present)
    */
   if (rstream_version == 0) {
     float read_alt = gbfgetflt(file_in);
-    char buf[32];
-    sprintf(buf, "%15.10f", read_alt);
-    // test for both cases to avoid compiler differences
-    if ((strcmp(buf, "-nan") == 0) || (strcmp(buf, "nan") == 0)) {
+    if (std::isnan(read_alt)) {
       wpt_tmp->altitude = unknown_alt;
     } else if (METERS_TO_FEET(wpt_tmp->altitude) <= -10000) {
       wpt_tmp->altitude = unknown_alt;
@@ -1169,7 +1166,10 @@ static void
 lowranceusr4_parse_route()
 {
   int route_version;
-  int UUID1, UUID2, UUID3, UUID4;
+  int UUID1 = 0;
+  int UUID2 = 0;
+  int UUID3 = 0;
+  int UUID4 = 0;
 
   lowranceusr4_fsdata* fsdata = lowranceusr4_alloc_fsdata();
   fs_chain_add(&(rte_head->fs), (format_specific_data*) fsdata);

diff --git a/mapsource.cc b/mapsource.cc
index 54d4a7b41e547ba983a54dd0e285a00e8d5d301e..7ba2c283ff6d6d3dc4d2aa9d512bc683b26c2948 100644 (file)
--- a/mapsource.cc
+++ b/mapsource.cc
@@ -312,7 +312,8 @@ static void
 mps_readstr(gbfile* mps_file, char* buf, size_t sz)
 {
   int c;
-  while (sz-- && (c = gbfgetc(mps_file)) != EOF) {
+  buf[sz-1] = 0;
+  while (--sz && (c = gbfgetc(mps_file)) != EOF) {
     *buf++ = c;
     if (c == 0)  {
       return;

diff --git a/pcx.cc b/pcx.cc
index 8649b5e58f4a19c58c7b0cc4d32e569fdf31dfd2..e982d18a157d0c847a1938dd2d12a93cb9dfbfe0 100644 (file)
--- a/pcx.cc
+++ b/pcx.cc
@@ -117,7 +117,7 @@ static void data_read() {
       case 'W': {
         QStringList tokens =
             line.split(QRegExp("\\s+"), QString::KeepEmptyParts);
-        if (tokens.size() < 5) {
+        if (tokens.size() < 6) {
           fatal(MYNAME
                 ": Unable to parse waypoint, not all required columns "
                 "contained\n");
@@ -217,7 +217,7 @@ static void data_read() {
       case 'T': {
         QStringList tokens =
             line.split(QRegExp("\\s+"), QString::KeepEmptyParts);
-        if (tokens.size() < 5) {
+        if (tokens.size() < 6) {
           fatal(MYNAME
                 ": Unable to parse trackpoint, not all required columns "
                 "contained\n");